Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use try...catch block with underscore.js template. #5984

Merged
merged 6 commits into from
Jul 31, 2019
Merged

Use try...catch block with underscore.js template. #5984

merged 6 commits into from
Jul 31, 2019

Conversation

dojutsu-user
Copy link
Member

#5980 doesn't fixes #5978
Turns out that the problem was not of variable declaration.
But the error was due to syntax changes in underscore.js templating engine.

See: https://stackoverflow.com/a/25881231/8601393

@dojutsu-user dojutsu-user requested a review from a team July 24, 2019 08:43
Copy link
Contributor

@davidfischer davidfischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine, but I want a bit more details in the PR and in the comments.

// because of change of syntax in new versions.
// See: https://stackoverflow.com/a/25881231/8601393
try {
// this is the old syntax
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's be a little more specific in the comment here. Sphinx bundles Underscore.js v1.3.1 (and has for many years) and it looks like this syntax changed in Underscore v1.7.0 (not exactly how semver is supposed to work...). So Sphinx should always use the old syntax and almost never the new syntax. How was this throwing a syntax error exactly? Were people somehow bundling newer versions of Underscore?

Copy link
Contributor

@davidfischer davidfischer Jul 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I figured it out. Sphinx has bundled Underscore.js v1.3.1 as I mentioned. However, doc builds before ~2019 used centrally located versions of jQuery and Underscore.js (due to readthedocs/readthedocs-sphinx-ext#52) as an optimization. That centrally located one uses Underscore.js v1.7.0. This optimization was removed precisely because it caused version mismatch issues like this! However, old doc builds still use this optimization.

Copy link
Contributor

@davidfischer davidfischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made one comment suggestion for clarity. Otherwise, this is good.

readthedocs/core/static-src/core/js/doc-embed/search.js Outdated Show resolved Hide resolved
Co-Authored-By: David Fischer <[email protected]>
@agjohnson
Copy link
Contributor

Is it okay that we're assuming that underscore is loaded by the page? Is underscore a new implicit dependency with this change?

The only case I can think of where a user does not have underscore.js is if they have a heavily customized theme -- which is more of an issue on our commercial hosting.

@dojutsu-user
Copy link
Member Author

@agjohnson
I think that it is okay to assume this.
I am not sure of this, but I believe sphinx adds underscore.js in every docs.
I have one example -- https://docs.signalfx.com/en/latest/
This is a heavily customised theme and it does includes underscore.js.

@agjohnson
Copy link
Contributor

My point was I thought we needed to verify this, we've postulated about it a good deal. I inspected our commercial hosting, where themes are more heavily customized, and the only projects that did not have underscore.js were mkdocs projects. Sphinx adds these files if they are referenced, but a heavily customized theme might not trigger this reference. An implicit dependency seems okay here. 👍

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 once the conflict is resolved.

@dojutsu-user
Copy link
Member Author

@ericholscher
This is mergeable now.

@davidfischer
Copy link
Contributor

The only case I can think of where a user does not have underscore.js is if they have a heavily customized theme -- which is more of an issue on our commercial hosting.

While it is possible, Sphinx search doesn't work without Underscore.js. So if they've customized so heavily that they've removed Underscore, they've likely also removed search.

@ericholscher ericholscher merged commit a7cd4e5 into readthedocs:master Jul 31, 2019
@dojutsu-user dojutsu-user deleted the fix-js-file-again branch July 31, 2019 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search not working
4 participants